Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(crashlytics): deprecation warning for v8 API ahead of future major release #8107

Merged
merged 9 commits into from
Nov 22, 2024

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Oct 31, 2024

Description

  • A little bit tricky logging console warning as we're deprecating a method that is also called by the modular method we're not deprecating.
  • Tested console.warn()'s are called for v8 API as opposed to v9 API.
  • Deprecated the method for isCrashlyticsCollectionEnabled as I think it should just be a property on the Crashlytics instance.
  • Created a separate branch so we can merge all changes in there before merging with main branch.
  • This is the first step towards moving completely to v9 API. I'll also be sweeping up any API that doesn't match Firebase web JS v9 API whether deprecating/creating to match.

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

Copy link

vercel bot commented Oct 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 2:47pm

@russellwheatley russellwheatley changed the base branch from main to v8-deprecations October 31, 2024 17:56
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍿 ! "and so it begins..."

I wonder though - this seems like a lot of repetition required in 2x the places (alters modular and old-style implementations), and 2x the work since there will be removal required in the modular API when deprecation complete

Related thought: the modular implementation will eventually host the actual implementation, inverting the current situation where old-style has the implementation and modular delegates, that is a necessary change that is unavoidable when removing the old-style implementation

So with this style it is:
1- add deprecation machinery to modular and old-style (2 spots)
2- move implementation from old-style to modular (unavoidable)
3- remove old-style and deprecation machinery from modular (2 spot)

I wonder if re-ordering it would be more efficient, and lead to cleaner deprecation addition:

1- move implementation from old-style to modular (unavoidable anyway)
2- add deprecation to old-style (only 1 spot, doesn't pollute modular)
3- remove old-style (only 1 spot)

🤔

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the boilerplate here bothered me (though I thought the idea was fine after discussion, and when I saw it again on #8132 I tried #8135 which seemed to work

If #8135 seems okay, then merging that one first, then using it in this one would make this diff tiny I think

@russellwheatley russellwheatley merged commit 5e5072d into v8-deprecations Nov 22, 2024
14 of 15 checks passed
@russellwheatley russellwheatley deleted the crashlytics-deprecations branch November 22, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants